Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(docs): add workflow to generate documentation on PR merge #12681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

virajbhartiya
Copy link
Member

@virajbhartiya virajbhartiya commented Nov 7, 2024

Related Issues

Closes #12233

Proposed Changes

Add a workflow that run make docsgen-cli whenever the PR is merged

Checklist

.github/workflows/check.yml Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Nov 7, 2024

@virajbhartiya can you resolve the conflict with master here please? You'll see that your " substitutions are getting in the way, so resolving those would be good.

@virajbhartiya
Copy link
Member Author

Hey @rvagg, I have resolved the conflicts in the PR

@virajbhartiya
Copy link
Member Author

@rvagg any suggestions on why is the workflow failing?

@rvagg
Copy link
Member

rvagg commented Nov 18, 2024

Failing because you need to install system dependencies for the build

in check.yml, you'll see:

      - uses: ./.github/actions/install-system-dependencies
      - uses: ./.github/actions/install-go
      - uses: ./.github/actions/make-deps

these are required to perform the actions you need, so you'll have to do it again in your new job

but, having said that, it's not cheap, so maybe this should all be in a single job so that's all done once and it's just the docsgen-cli bit that's done separately.

So how about this:

  1. rename check.yml to check-and-gen.yml
  2. add a docsgen-cli section to it with your new stuff, so that happens in the same flow

I'm still not convinced this is a great idea, it might get in the way. So we'll need to have a collective chat with others before we proceed.

@virajbhartiya
Copy link
Member Author

Hey @rvagg thanks for the review, I have currently modified it accordingly, renamed check.yml to check-and-gen.yml and added it in the same file. We can discuss regarding this on slack wtih other how to go ahead with this PR

@BigLep
Copy link
Member

BigLep commented Nov 25, 2024

I know there have been verbal conversation on this one. Did we write out the next steps?

@rvagg
Copy link
Member

rvagg commented Nov 26, 2024

Didn't write it out, but next steps are to get this completed and merged and then experience it live and decide whether we hate it or love it and whether it's a QoL improvement on balance.

Copy link
Contributor

@galargh galargh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, https://github.com/ipdxco/github-as-code/blob/master/.github/workflows/fix.yml is a working example of a workflow that runs on PRs and writes to PR branches.

.github/workflows/check-and-gen.yml Outdated Show resolved Hide resolved
.github/workflows/check-and-gen.yml Outdated Show resolved Hide resolved
@BigLep
Copy link
Member

BigLep commented Jan 21, 2025

@rvagg : do you think we can do the next steps in #12681 (comment) this week to run the experiment? (Or have new thoughts come in and we defer for now.)

@BigLep
Copy link
Member

BigLep commented Jan 21, 2025

@virajbhartiya : is the final feedback something you'll be able to incorporate or does someone else need to take this?

@rvagg rvagg force-pushed the docsgen-workflow branch 4 times, most recently from ebcb89a to dfc31a3 Compare January 29, 2025 01:56
@rvagg
Copy link
Member

rvagg commented Jan 29, 2025

I hope you don't mind @virajbhartiya but I've squashed this down to one commit, rebased on master, added a little fix to make sure this is only done on PRs and have force pushed all of that to your branch

@rvagg
Copy link
Member

rvagg commented Jan 29, 2025

#12854

@rvagg
Copy link
Member

rvagg commented Jan 29, 2025

OK, I'm pushing my latest attempt at this and giving up for now, it's driving me mad.

  • chore: break docsgen-cli to see if it gets generated for me #12856 is a PR testing it out, and that's just from this repo, I gave up on testing it from a fork but that would need to be done too.
  • It makes a merge commit, I think because actions/checkout does a detached head but I thought ref: was supposed to deal with that, argh!
  • The push after commit wipes out all of the "Checks" so to find the actions runs you have to hunt through the commits and look for the ❌ or ✅ and get to it via that; this isn't OK and would be cause enough to bail on this whole project if this is what you have to put up with if an action pushes to a branch that it's checking.
  • I really wanted to see a comment on the PR come out of it but didn't get past all the other frustration to make my commenter work, but that's probably not too hard, see error in https://github.com/filecoin-project/lotus/actions/runs/13024797399/job/36331973937?pr=12856

As of now, @virajbhartiya's docsgen-workflow branch has my latest. If this doesn't get picked up again soon I think we should just give up. docsgen-cli is much faster now than it once was and it may not be a bad thing to even bundle it with docsgen. Asking the user to do it is less onerous than it was when we started this.

@BigLep
Copy link
Member

BigLep commented Feb 4, 2025

@galargh : if you are up for timeboxing 1 hour to land this, that's great. I wouldn't want to spend more than an hour here though as we are ok to close it too.

@galargh
Copy link
Contributor

galargh commented Feb 4, 2025

@galargh : if you are up for timeboxing 1 hour to land this, that's great. I wouldn't want to spend more than an hour here though as we are ok to close it too.

This is a minimal example of a workflow committing to pull requests (including public, non-org forks where the user allowed maintainer edits) - https://github.com/galorgh/.github/blob/master/.github/workflows/playground3.yml

It requires the use of pull_request_target to obtain credentials that are allowed writing to forks. One should be very careful when operating the the pull request target context because it has access to secrets and powerful credentials.

The pushed commit doesn't trigger any more workflows by design. This means it is not suitable for repos with required checks.

The way to overcome the limitations of this approach is to use custom credentials - either bot account's PAT or designated GItHub App's secrets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ⌨️ In Progress
Development

Successfully merging this pull request may close these issues.

Run make docsgen-cli on merge
5 participants